Conversation
🛡️ Bandit Security Scan Results✅ No security issues found by Bandit. |
There was a problem hiding this comment.
Pull request overview
This PR establishes a comprehensive test suite for the Codesphere Python SDK, including both unit and integration tests. The changes refactor the SDK's internal architecture to support lazy operation loading (avoiding circular imports), add extensive test coverage across all resource types, and provide integration test infrastructure with automated workspace setup/cleanup.
Changes:
- Set up complete test infrastructure with unit and integration tests covering Teams, Workspaces, Domains, Environment Variables, and Metadata resources
- Refactored SDK internals to use lazy operation loading via
_execute_operation()method, eliminating circular import issues - Added integration test workflow with session-scoped test workspace management and environment variable configuration
- Updated documentation with testing guidelines and separated command schemas to resolve import cycles
Reviewed changes
Copilot reviewed 70 out of 87 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_client.py | Migrated client tests to class-based structure with improved fixtures |
| tests/resources/workspace/test_workspace.py | Added comprehensive workspace resource unit tests |
| tests/resources/workspace/env_vars/test_env_vars.py | Added environment variables manager unit tests |
| tests/resources/team/test_team.py | Added team resource unit tests |
| tests/resources/team/domain/test_domain.py | Added domain resource unit tests |
| tests/resources/metadata/test_metadata.py | Added metadata resource unit tests |
| tests/integration/test_*.py | Added integration tests for all major resources |
| tests/integration/conftest.py | Added session-scoped workspace fixtures for integration tests |
| tests/conftest.py | Added shared test fixtures and mock factories |
| tests/resources/conftest.py | Added resource-specific test fixtures |
| src/codesphere/resources/workspace/schemas.py | Refactored to use lazy operation loading and separated command schemas |
| src/codesphere/resources/workspace/command_schemas.py | Extracted command-related schemas to avoid circular imports |
| src/codesphere/resources/workspace/envVars/schemas.py | Separated EnvVar schema to avoid circular imports |
| src/codesphere/resources/workspace/envVars/models.py | Refactored to use lazy operation loading |
| src/codesphere/resources/metadata/schemas.py | Added explicit Field validation aliases for CPU/RAM/etc fields |
| src/codesphere/core/handler.py | Enhanced null checks and removed debug logging |
| src/codesphere/core/base.py | Added serialize_by_alias configuration |
| src/codesphere/config.py | Added extra="ignore" to allow additional CS_* env vars |
| Makefile | Added test-unit and test-integration targets |
| .github/workflows/integration.yml | Added integration test GitHub Actions workflow |
| CONTRIBUTING.md | Added comprehensive testing guidelines section |
| examples/* | Removed example files (likely to be regenerated) |
Pytest Coverage Report
Test Execution Summary
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Integration Test Results26 tests 26 ✅ 50s ⏱️ Results for commit ac2df3c. ♻️ This comment has been updated with latest results. |
| default=_BULK_SET_OP, | ||
| exclude=True, | ||
| ) | ||
| return await self._execute_operation(_GET_OP) |
There was a problem hiding this comment.
WorkspaceEnvVarManager.get() is annotated to return List[EnvVar] but it returns the raw operation result. If _GET_OP returns ResourceList[EnvVar] (consistent with other list endpoints), callers will receive a ResourceList instead of a list. Consider mirroring other resources: result = await ...; return result.root (or change the return annotation/docstring to ResourceList[EnvVar]). This also makes the behavior consistent with TeamsResource.list() / WorkspacesResource.list().
| return await self._execute_operation(_GET_OP) | |
| result: ResourceList[EnvVar] = await self._execute_operation(_GET_OP) | |
| return result.root |
| async def set( | ||
| self, env_vars: Union[ResourceList[EnvVar], List[Dict[str, str]]] | ||
| ) -> None: |
There was a problem hiding this comment.
The type hints for set()/delete() don’t include List[EnvVar], but the unit tests call these methods with lists of EnvVar instances. Updating the annotations to include List[EnvVar] (and possibly broadening to a Sequence[...]) will align the public API with actual supported usage and avoid confusing type-checker errors.
| ) | ||
| await self._execute_operation(_BULK_SET_OP, data=payload.model_dump()) | ||
|
|
||
| async def delete(self, items: Union[List[str], ResourceList[EnvVar]]) -> None: |
There was a problem hiding this comment.
The type hints for set()/delete() don’t include List[EnvVar], but the unit tests call these methods with lists of EnvVar instances. Updating the annotations to include List[EnvVar] (and possibly broadening to a Sequence[...]) will align the public API with actual supported usage and avoid confusing type-checker errors.
| def mock_http_client_for_resource(mock_response_factory): | ||
| """ | ||
| Create a configurable mock HTTP client for resource testing. | ||
|
|
||
| Returns a factory function that creates configured mock clients. | ||
| """ | ||
|
|
||
| def _create(response_data: Any, status_code: int = 200) -> MagicMock: | ||
| mock_client = MagicMock() | ||
| mock_response = mock_response_factory.create( | ||
| status_code=status_code, | ||
| json_data=response_data, | ||
| ) | ||
| mock_client.request = AsyncMock(return_value=mock_response) | ||
| return mock_client | ||
|
|
||
| return _create | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def teams_resource_factory(mock_http_client_for_resource): | ||
| """Factory for creating TeamsResource instances with mock data.""" | ||
|
|
||
| def _create(response_data: Any): | ||
| from codesphere.resources.team import TeamsResource | ||
|
|
||
| mock_client = mock_http_client_for_resource(response_data) | ||
| resource = TeamsResource(http_client=mock_client) | ||
| return resource, mock_client | ||
|
|
||
| return _create | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def workspaces_resource_factory(mock_http_client_for_resource): | ||
| """Factory for creating WorkspacesResource instances with mock data.""" | ||
|
|
||
| def _create(response_data: Any): | ||
| from codesphere.resources.workspace import WorkspacesResource | ||
|
|
||
| mock_client = mock_http_client_for_resource(response_data) | ||
| resource = WorkspacesResource(http_client=mock_client) | ||
| return resource, mock_client | ||
|
|
||
| return _create | ||
|
|
||
|
|
||
| @pytest.fixture |
There was a problem hiding this comment.
This fixture (and several factories below it) duplicates implementations already present in tests/conftest.py with the same fixture names. Keeping two versions increases the chance of behavior drift and makes it harder to know which fixture is used (pytest will prefer the closest conftest.py). Consider reusing the root fixtures instead (e.g., remove the duplicates here, or rename these to resource-specific names and delegate to the shared factory).
| def mock_http_client_for_resource(mock_response_factory): | |
| """ | |
| Create a configurable mock HTTP client for resource testing. | |
| Returns a factory function that creates configured mock clients. | |
| """ | |
| def _create(response_data: Any, status_code: int = 200) -> MagicMock: | |
| mock_client = MagicMock() | |
| mock_response = mock_response_factory.create( | |
| status_code=status_code, | |
| json_data=response_data, | |
| ) | |
| mock_client.request = AsyncMock(return_value=mock_response) | |
| return mock_client | |
| return _create | |
| @pytest.fixture | |
| def teams_resource_factory(mock_http_client_for_resource): | |
| """Factory for creating TeamsResource instances with mock data.""" | |
| def _create(response_data: Any): | |
| from codesphere.resources.team import TeamsResource | |
| mock_client = mock_http_client_for_resource(response_data) | |
| resource = TeamsResource(http_client=mock_client) | |
| return resource, mock_client | |
| return _create | |
| @pytest.fixture | |
| def workspaces_resource_factory(mock_http_client_for_resource): | |
| """Factory for creating WorkspacesResource instances with mock data.""" | |
| def _create(response_data: Any): | |
| from codesphere.resources.workspace import WorkspacesResource | |
| mock_client = mock_http_client_for_resource(response_data) | |
| resource = WorkspacesResource(http_client=mock_client) | |
| return resource, mock_client | |
| return _create | |
| @pytest.fixture | |
| @pytest.fixture |
| def test_client_initialization(self, api_http_client, mock_token): | ||
| """Client should initialize with token from settings.""" | ||
| assert api_http_client._token == mock_token |
There was a problem hiding this comment.
The test suite no longer covers the negative initialization path (e.g., missing/empty token leading to an authentication/config error). Adding a test for the failure case would improve coverage of client setup and prevent regressions in the settings/token handling logic.
1 participant
No description provided.